fix: SessionEnd/status lookups miss state written under a different CLAUDE_PLUGIN_DATA root, orphaning brokers - #659

…LAUDE_PLUGIN_DATA root, orphaning brokers

resolveStateDir() picks the state root from CLAUDE_PLUGIN_DATA when
set, falling back to $TMPDIR/codex-companion when it's absent -- same
workspace slug/hash either way, only the root differs. State written
under one root (e.g. a broker registered while the var was unset)
becomes invisible to any later lookup that resolves to the other
root, since nothing checked both. For a broker specifically, that
means SessionEnd never finds it to shut down -- it's orphaned
permanently, and since ensureBrokerSession also can't see it, the
next session spawns a duplicate broker for the same workspace,
compounding the leak. The same mechanism affects job/status state
(state.json, individual job detail files), not just the broker.

Reads now check every candidate root (current primary, then the
tmpdir fallback), not just the current invocation's primary --
writes are unchanged, still going to the primary root. Applied to
loadState() (job list, status, config), readStoredJob() (individual
job detail lookups), and loadBrokerSession()/clearBrokerSession().

Known remaining asymmetry: this fixes the direction with concrete
evidence in the issue -- state written while CLAUDE_PLUGIN_DATA was
unset, later missed by a lookup that has it set. The reverse isn't
fixable this way: an unset env var carries no trace of what value it
previously held, so there's nothing to check beyond the always-known
tmpdir fallback.

Fixes openai#636

…turned

Both call sites (handleSessionEnd, ensureBrokerSession) act on
whatever loadBrokerSession() returns -- tearing that broker down and
clearing its record -- but clearBrokerSession deleted every candidate
root's broker.json, not just the one that was actually torn down.

That's reachable in practice: it's precisely the root-split bug's own
historical fallout, where the old lookup could leave a broker
registered under one root while a duplicate got spawned under the
other. Deleting both records on the next cleanup erases the untorn
broker's only metadata, making it permanently untrackable instead of
leaving a stale-but-discoverable file behind.

Thanks to Codex Review for catching this.

… with loadBrokerSession on malformed files

Two more issues from Codex Review, both real:

1. loadState() returned only the first candidate state.json found,
   not merged. Unlike a broker session (at most one meaningful
   record, so 'first found' is correct), jobs are a growing
   collection -- a job started while CLAUDE_PLUGIN_DATA was set and a
   different job started while it was unset are both real and
   non-conflicting. Returning only the first root's job list silently
   hid whichever root wasn't picked, for every status/result/cancel
   lookup, any time both roots happened to have a state.json. Now
   merges jobs from every candidate, keeping the more recently
   updated copy if the same id somehow appears in more than one.

2. loadBrokerSession() skips a candidate it can't parse and moves on,
   so it can return a fallback session while a malformed primary file
   exists. clearBrokerSession() selected by existence alone, so it
   could delete the unrelated malformed primary while leaving the
   valid fallback record behind -- the one actually loaded and torn
   down by the caller. Both functions now share a single
   selectBrokerState() helper (exists AND parses), so they always
   agree on which candidate is the selected one.

Verified the loadState() merge fix doesn't have a side effect on
saveState()'s own previousJobs cleanup diff (its per-job file removal
resolves paths against the current-root-only resolveJobFile(), so a
job living in another root is a no-op there, not a deletion) --
confirmed empirically with a throwaway repro before concluding no
further change was needed there.

Thanks again to Codex Review.

…t the primary

saveState() only ever wrote the new job list to the current primary
root. A job that originated entirely in a different root (e.g. added
while CLAUDE_PLUGIN_DATA was unset) and later gets filtered out --
cleanupSessionJobs() during SessionEnd loads the merged view, drops
jobs for the ending session, and saves the remainder -- never
actually disappeared: that other root's own state.json still held
its own untouched copy, and the very next loadState() merged it right
back in. A removed job could keep reporting as running indefinitely.

saveState() now also prunes every other candidate root's own file
down to the same retained job-id set (derived from this save's own
merged previousJobs diff), so a deletion sticks everywhere. New and
updated jobs are unaffected -- they still only ever get written to
the primary root, exactly as before; this only ever removes.

Also made the individual job-detail-file cleanup in the same loop
candidate-aware (resolveJobFileCandidates instead of the
primary-only resolveJobFile), for the same reason.

Two of the existing tests had to seed their two-root fixtures via
direct file writes instead of two independent saveState() calls --
every real caller (updateState()/cleanupSessionJobs()) always derives
its job list from a prior loadState(), so seeding via two disjoint,
non-full-list saveState() calls doesn't reflect any real call
pattern, and (correctly, now) tripped this very fix's own deletion
logic during test setup.

Thanks again to Codex Review.